Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add extended tracer module #759

Merged
merged 33 commits into from
Nov 8, 2023
Merged

add extended tracer module #759

merged 33 commits into from
Nov 8, 2023

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Feb 23, 2023 •
edited
Loading

The Opentelemetry APIs are rather difficult to use - therefore I've developed the following utility method, which is part of a company internal library - so it has already been tested by some users.

Based on this PR, I'm now creating a PR here.

@zeitlinger zeitlinger requested a review from a team February 23, 2023 16:37
@zeitlinger zeitlinger mentioned this pull request Feb 23, 2023
Closed
@zeitlinger
Copy link
Member Author

@trask I hope this PR makes more sense now - I've kept the idea of being able to wrap a Tracer - but I've also kept the static methods in the Tracing class.

trask
trask reviewed Jul 20, 2023
View reviewed changes
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little hesitant to introduce too much syntactic sugar on top of opentelemetry-api, since that creates more ways to do the same thing, which could cause confusion

at the same time, it makes sense to have somewhere to experiment with things that we might want to add into the opentelemetry-api itself

we also have opentelemetry-instrumentation-api, which possibly has a little bit of overlap

I left a couple of initial comments, overall I think it might help to pair the PR down and just focus on the Tracing class at least initially

extended-tracer/src/main/java/io/opentelemetry/contrib/tracer/CurrentSpan.java Outdated Show resolved Hide resolved
extended-tracer/src/main/java/io/opentelemetry/contrib/tracer/Tracing.java Outdated Show resolved Hide resolved
extended-tracer/src/main/java/io/opentelemetry/contrib/tracer/Tracing.java Outdated Show resolved Hide resolved
extended-tracer/src/main/java/io/opentelemetry/contrib/tracer/Tracing.java Outdated Show resolved Hide resolved
@zeitlinger zeitlinger force-pushed the extended-tracer branch 2 times, most recently from 1326088 to 150b2be Compare July 27, 2023 14:18
@zeitlinger
Copy link
Member Author

we also have opentelemetry-instrumentation-api, which possibly has a little bit of overlap

I checked Instrumenter but didn't see anything that could be reused.

I left a couple of initial comments, overall I think it might help to pair the PR down and just focus on the Tracing class at least initially

I like that idea and have adjusted the PR accordingly 😄

@zeitlinger
Copy link
Member Author

@trask @jkwatson @jack-berg as discussed yesterday, I've removed GlobalTracer, injected Tracer into Tracing, and added before/after examples.

This was referenced Aug 26, 2023
Closed
Closed
zeitlinger and others added 24 commits November 7, 2023 11:10
@zeitlinger
- remove GlobalTracing.java for now (discussion pending)
176f116 
- inject Tracer into Tracing, making it easier to use
@zeitlinger
add before/after comparison for extended tracer
9619428
@zeitlinger
add before/after comparison for extended tracer
fa0a83a
@zeitlinger
- catch & re-throw Throwable over Exception everywhere
8e348cc 
- clarify error handling strategy
@zeitlinger
add javadoc
5cbb7f8
@zeitlinger
take span builder instead of span to reinforce that the span is start…
db2263d 
…ed and ended by run/call
@zeitlinger
use throwing supplier which avoids using sneaky throws
6b35e78
@zeitlinger
remove non-essential parts of Tracing
9c0cd7a
@zeitlinger @trask
Update extended-tracer/src/main/java/io/opentelemetry/contrib/tracer/…
8c59ebe 
…Tracing.java

Co-authored-by: Trask Stalnaker <[email protected]>
@zeitlinger @trask
Update extended-tracer/build.gradle.kts
5c948ba 
Co-authored-by: Trask Stalnaker <[email protected]>
@zeitlinger @trask
Update extended-tracer/build.gradle.kts
13c3819 
Co-authored-by: Trask Stalnaker <[email protected]>
@zeitlinger @trask
Update extended-tracer/src/main/java/io/opentelemetry/contrib/tracer/…
12dccfa 
…ThrowingSupplier.java

Co-authored-by: Trask Stalnaker <[email protected]>
@zeitlinger
rename ThrowingSupplier to SpanCallback
c060404
@zeitlinger @trask
Update extended-tracer/src/main/java/io/opentelemetry/contrib/tracer/…
b0500ba 
…Tracing.java

Co-authored-by: Trask Stalnaker <[email protected]>
@zeitlinger
rename transport to carrier
bb9e704
@zeitlinger
add SpanRunnable similar to SpanCallback
afbefa0
@zeitlinger
rename propagation methods
a264436
@zeitlinger
add SpanRunnable similar to SpanCallback
18ed800
@zeitlinger
add more tests to show usage
ad1b988
@zeitlinger
- make callWithBaggage static
7cd4616 
- extract Propagation class with static methods
@zeitlinger
- make callWithBaggage static
68e1b5c 
- extract Propagation class with static methods
@zeitlinger
- make callWithBaggage static
07e55dd 
- extract Propagation class with static methods
@zeitlinger @trask
Update extended-tracer/build.gradle.kts
c6e0b9c 
Co-authored-by: Trask Stalnaker <[email protected]>
@zeitlinger @trask
Update extended-tracer/build.gradle.kts
11f07bf 
Co-authored-by: Trask Stalnaker <[email protected]>
@zeitlinger
Copy link
Member Author

@trask build is passing now 😄

@trask trask merged commit 2983ea7 into open-telemetry:main Nov 8, 2023
@trask
Copy link
Member

trask commented Nov 8, 2023

thanks @zeitlinger for your patience and incorporating all the feedback!

@zeitlinger zeitlinger deleted the extended-tracer branch November 8, 2023 17:29
@trask trask mentioned this pull request Dec 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PeterF778 PeterF778 PeterF778 left review comments

@jkwatson jkwatson jkwatson left review comments

@kenfinnigan kenfinnigan kenfinnigan left review comments

@trask trask trask approved these changes

@breedx-splk breedx-splk breedx-splk approved these changes

@jack-berg jack-berg jack-berg approved these changes

Assignees

@dehaansa dehaansa

@zeitlinger zeitlinger

@iNikem iNikem

@oertl oertl

@rmfitzpatrick rmfitzpatrick

@Mrod1598 Mrod1598

@jeanbisutti jeanbisutti

@LikeTheSalad LikeTheSalad

@breedx-splk breedx-splk

@PeterF778 PeterF778

Labels
None yet
Projects
None yet
Milestone
v1.32.0
Development

Successfully merging this pull request may close these issues.